Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate CBs and Sems to kernel config ring buffer #11595

Closed
wants to merge 6 commits into from

Conversation

pgkeller
Copy link
Contributor

Ticket

#4984

Problem description

Migration of sems and cb configs to ring buffer

What's changed

Move semaphores and CB configs to ring buffer.
Had to hack disable code in cq_dispatch due to code issues
Had to hack around race in llks (this code sped things up exposing a race, added a delay to compensate)

Checklist

  • Post commit CI passes
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

@@ -52,7 +52,7 @@
#define MEM_BOOT_CODE_BASE 0
#define MEM_L1_BARRIER 12
#define MEM_MAILBOX_BASE 16
#define MEM_MAILBOX_END (MEM_MAILBOX_BASE + 1344)
#define MEM_MAILBOX_END (MEM_MAILBOX_BASE + 1348)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we calculate this value? Could you add comment or make it depend on sum of other defines directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see below

uint32_t tt_l1_ptr *cb_l1_base = (uint32_t tt_l1_ptr *)(kernel_config_base +
mailboxes->launch.kernel_config.cb_offset);
setup_cb_read_write_interfaces(cb_l1_base, 0, mailboxes->launch.kernel_config.max_cb_index, cb_init_read, cb_init_write, cb_init_write);
#if defined(UCK_CHLKC_UNPACK)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment about why this delay was added with the issue link?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, will do

Copy link
Contributor

@tt-aho tt-aho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to run some model ci, ex perf/device perf, to double check perf/also ensure the race workaround works for those tests

Only include ierisc when needed, sometimes the values are 0
Dispatcher is out of memory on ncrisc, disabling for now
Removing the uneeded CB config inits sped up unpacker exposing an as yet
unknown timing issue.  This delay mimics the delay of the CB config inits
to keep tests passing
Trisc FW initializes CBs, this we being done again in llks, removed
@pgkeller
Copy link
Contributor Author

Closing. Semaphore change ran into major issues w/ T3k tests, fixed in a separate branch, will pull those in isolation then rebase the rest of this work on that.

@pgkeller pgkeller closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants